Minor fix to Window.Java. #3192
Conversation
There was a problem hiding this comment.
Thank you for the proposal. The code indeed looks wrong. It just seems to work because this method is only called at a single place where this.shell is passed as loopShell, such that both values are effectively always the same when reaching this line of code.
So I guess this is something you found by accident, but you did not experience any issues because of this, did you?
To improve this correction, I propose to do either of the following:
- Make the method also static, as that's what it effectively is
- Remove the
loopShellparameter and directly referenceshell
In addition to those, I propose to remove the null-check (and instead clarify in the contract that the argument is assumed to be non-null). This contract is currently fulfilled by the sole consumer of the method anyway, so no need for unnecessary null-checks and unused fallback mechanisms.
|
I did in fact find this by accident while focusing on another issue! I also did not encounter any issues because of this. I went ahead and implemented your corrections/proposal, check it out and let me know if everything is in check 🤓 Thank you for checking out my proposal as well as the suggestions @HeikoKlare |
grissinibread
force-pushed
the
master
branch
2 times, most recently
from
2cb597b to
2821e0a
Compare
bundles/org.eclipse.jface/src/org/eclipse/jface/window/Window.java
Outdated
Show resolved
Hide resolved
…ter and directly referenced shell. Updated the runEventLoop() contract for clarification.
Incorrect shell was being called in initial if statement.
Corrected to 'loopShell' which is the parameter created for this function.